fix: stop duplicate detection dropping distinct packets that share a 64-byte prefix - #882
Open
Chessing234 wants to merge 2 commits into
Open
fix: stop duplicate detection dropping distinct packets that share a 64-byte prefix#882Chessing234 wants to merge 2 commits into
Chessing234 wants to merge 2 commits into
Conversation
Replay and duplicate detection keyed on a 32-bit contentHashCode over at most the first 64 bytes of the payload. Two packets from the same peer in the same millisecond that agreed on that prefix were the same packet as far as this cache was concerned, and a collision here is a dropped message: the second is discarded and nothing reports it. PacketIdUtil is the identity the rest of the stack already uses for this question — gossip sync membership, and the message IDs MessageHandler assigns — and iOS derives it identically: first 16 bytes of SHA-256 over type, senderID, timestamp and the whole payload. The security path now agrees with the sync path instead of carrying a weaker private notion of "same packet", and the FRAGMENT special case disappears because the full payload is covered either way. Peer scoping is deliberately kept. PacketIdUtil covers the packet's own senderID, which is not the peer it arrived from once relayed.
The collision case fails on main: two packets sharing a 64-byte prefix and a timestamp, where the second was silently dropped. The other two pass before and after on purpose. Replay of an identical packet must still be caught, and the same packet arriving from two different peers must still be tracked separately — strengthening the identity must not quietly weaken either.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replay and duplicate detection in
SecurityManagerkeyed on this:A 32-bit
contentHashCodeover at most the first 64 bytes of the payload. Two packets from the same peer in the same millisecond that agree on that prefix are the same packet as far as this cache is concerned, and a collision here is not a false alarm — it is a dropped message.validatePacketreturns false, the packet is discarded, and nothing anywhere reports that it happened.Why this is worth changing rather than tuning
The repo already has the right answer to "are these the same packet".
PacketIdUtil— first 16 bytes of SHA-256 over type, senderID, timestamp and the whole payload — is whatGossipSyncManageruses for sync membership and whatMessageHandleruses to assign message IDs. iOS derives it identically (bitchat/Sync/PacketIdUtil.swift), so it is also the cross-platform notion.So the security path was carrying its own weaker private definition while the strong one sat in the same source tree, already used for the same question. Using it makes the two agree, and the
FRAGMENTspecial case disappears on the way — it existed only because the general branch truncated at 64 bytes, and the full payload is covered either way now.Peer scoping is deliberately kept.
PacketIdUtilcovers the packet's ownsenderID, which is not the same thing as the peer it was received from once a packet has been relayed, so the key stays"$peerID-$packetId"rather than becoming the bare packet ID.What is not claimed
This is not a fix for a forged-packet attack.
validatePacketalready records only packets that pass signature verification, with a comment explaining exactly why, so an attacker cannot poison the cache against a peer without that peer's signature. What changes here is accidental collision between two honest packets, and the fact that the security layer and the sync layer no longer disagree about packet identity.Verification
Local run of the CI job (
testDebugUnitTest lintDebug, JDK 21). Counts from--rerun-taskson both sides so they are real full-suite runs, not incremental leftovers:SecurityManagerTest, 24 → 27 — and nothing else moved.main, which is the point: two packets sharing a 64-byte prefix and a timestamp, second one silently dropped.CI has not run. Fork PRs here sit at
action_requireduntil a maintainer approves the workflow, so the local run is the evidence, not a green check.